Repository navigation
Conversation
|
Deployed it to MacOS 26 laptop (backported relevant patches from the series to #!/usr/bin/env bash
set -euo pipefail
testDir=$(mktemp -d)
holder=
cleanup() {
if [[ -n "$holder" ]] && kill -0 "$holder" 2>/dev/null; then
kill "$holder" 2>/dev/null || true
wait "$holder" 2>/dev/null || true
fi
rm -f "$testDir/payload" "$testDir/hold"
rmdir "$testDir" 2>/dev/null || true
}
trap cleanup EXIT
holderBash=$(command -v bash)
printf 'gc-runtime-test-%s-%s\n' "$$" "$RANDOM" > "$testDir/payload"
root=$(/run/current-system/sw/bin/nix store add-file "$testDir/payload")
mkfifo "$testDir/hold"
echo "active-nix=$(/run/current-system/sw/bin/nix eval --impure --raw --expr builtins.nixVersion)"
echo "holder-bash=$holderBash"
echo "disposable-path=$root"
# The empty argument exercises parsing of empty argv entries. The FIFO keeps
# this unentitled, Nix-built Bash process alive without starting a child that
# inherits GC_RUNTIME_ROOT.
env -i GC_RUNTIME_ROOT="$root" \
"$holderBash" -c 'read -r value < "$1"' '' "$testDir/hold" &
holder=$!
sleep 1
kill -0 "$holder"
echo "delete-while-live:"
if /run/current-system/sw/bin/nix-store --delete "$root"; then
echo "FAIL: Nix deleted a runtime-rooted path"
exit 1
fi
test -e "$root"
echo "GOOD: deletion refused while holder was alive"
kill "$holder"
wait "$holder" 2>/dev/null || true
holder=
echo "delete-after-exit:"
/run/current-system/sw/bin/nix-store --delete "$root"
test ! -e "$root"
echo "PASS: Darwin environment runtime-root discovery works"
|
|
MacOS CI failure is in |
The runtime root scanner assumes the Linux procfs layout. Other platforms already use lsof as their fallback, so avoid probing an incompatible or optionally mounted procfs first. Assisted-by: Codex (gpt-5.6-sol, high)
Darwin currently falls back to lsof, which cannot discover store paths held only in process environments and is prohibitively slow. Use libproc and KERN_PROCARGS2 to match the runtime-root coverage available on Linux. Based on Lix commits c03de0d, 1437d3d, and 068f4b1. Fixes NixOS#3011. Fixes NixOS#13990. Co-authored-by: Artemis Tosini <me@artem.ist> Co-authored-by: Jade Lovelace <lix@jade.fyi> Co-authored-by: Lily Ballard <lily@ballards.net> Assisted-by: Codex (gpt-5.6-sol, high)
The libproc implementation provides the process, environment, and open-file root discovery covered by this test. Assisted-by: Codex (gpt-5.6-sol, high)
KERN_PROCARGS2 represents empty arguments as consecutive NUL bytes. Skip padding only after the executable, then advance exactly one entry per argument so environment roots are not missed. Strengthen gc-runtime to keep the environment root out of the GC process and cover an empty argument. Assisted-by: Codex (gpt-5.6-sol, high)
A thread may exit between listing it and querying its working directory. Ignore ESRCH for that thread so the remaining thread roots are still inspected. This is similar to how we gracefully continue when an fd is closed. Assisted-by: Codex (gpt-5.6-sol, high)
Document the new libproc scanner and the macOS restriction on reading environment variables from entitled processes. Assisted-by: Codex (gpt-5.6-sol, high)
04e456c to
938c019
Compare
| // and has been available since OS X 10.10, but is not exposed | ||
| // by the SDK headers. Its numeric value is 22. |
There was a problem hiding this comment.
It's safe to drop this hack then since we are in the clear by having nixpkgs only support 14 and upwards?
| if (e.is(std::errc::no_such_file_or_directory) || e.is(std::errc::no_such_process) | ||
| || e.is(std::errc::invalid_argument) || e.is(std::errc::permission_denied) | ||
| || e.is(std::errc::operation_not_permitted) || e.is(std::errc::io_error)) |
There was a problem hiding this comment.
Can we maybe extend the is overload set with a overload that takes a variadic template pack?
|
|
||
| tids.resize(tidBufSize / sizeof(uint64_t)); | ||
| for (auto tid : tids) { | ||
| struct proc_threadwithpathinfo threadPathInfo; |
There was a problem hiding this comment.
Any particular reason why we need the C-style struct suffix? Seems like it could be safely dropped. I'd also maybe opt for explicitly qualifying with ::proc_threadwithpathinfo but that's more of a keyboard heating thing than an actual issue in practice.
xokdvium
left a comment
There was a problem hiding this comment.
Thanks for pushing this. Haven't yet tested this on my mac, but looks promising indeed. Mostly stylistic questions.
| // File descriptors. | ||
| std::vector<struct proc_fdinfo> fds; | ||
| std::size_t fdBufSize = 1; | ||
| while (fdBufSize > fds.size() * sizeof(struct proc_fdinfo)) { | ||
| // Reserve some extra size so we don't fail too much. | ||
| fds.resize((fdBufSize + fdBufSize / 8) / sizeof(struct proc_fdinfo)); | ||
| errno = 0; | ||
| auto size = proc_pidinfo(pid, PROC_PIDLISTFDS, 0, fds.data(), fds.size() * sizeof(struct proc_fdinfo)); | ||
|
|
||
| // The libproc wrapper converts a -1 syscall result to 0. A | ||
| // process with no file descriptors also returns 0, but leaves | ||
| // errno unchanged. | ||
| if (size <= 0) { | ||
| if (errno == 0) { | ||
| fdBufSize = 0; | ||
| break; | ||
| } | ||
| throw SysError("listing pid %1% file descriptors", pid); | ||
| } | ||
| fdBufSize = size; | ||
| } | ||
| fds.resize(fdBufSize / sizeof(struct proc_fdinfo)); |
There was a problem hiding this comment.
Curious. I guess we could use this stuff in closeExtraFDs in process spawning machinery instead of the less efficient approach that it takes now.
| return boost::regex(quotedStoreDir + R"(/[0-9a-z]+[0-9a-zA-Z\+\-\._\?=]*)"); | ||
| } | ||
|
|
||
| void findDarwinRuntimeRoots(const StoreDirConfig & config, UncheckedRoots & unchecked); |
There was a problem hiding this comment.
I guess in the future we could also factor out Linux-specific code. Seems nice, though I'd probably rename this to something like findPlatformRuntimeRoots so that we could just have separate platform-specific files implementing this function.
Motivation
On Darwin, lsof was used to detect gcroots. This is bad:
It may result in gcroots removed while e.g. nix shell or nixpkgs vm with 9p
store is running.
For the latter, see also:
NixOS/nixpkgs#440579
Context
This is not happening on Lix because they switched to libproc circa 2024.
This PR largely adopts Lix implementation. With libproc, we can now read
envvars and hold gcroots mentioned there.
The Lix code is taken largely verbatim but was adopted to this codebase where
necessary. In addition, two bugs were identified in their implementation. Those
bugs are fixed in separate commits, for easier review and reference of Lix
folks in case they ever want to backport them.
The bugs are:
not consulted for possible gcroots;
search, which may result in removing active roots.
The PR re-enables a previously disabled test case on darwin.
lsof is left for other platforms. Also, - a drive-by change - /proc is no
longer crawled on non-Linux platforms since the specific /proc layout is not
portable. (The /proc walk did not fail, so it's only a minor optimization /
correctness fix.)
Fixes #3011
Fixes #13990
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.